-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docker #340
Docker #340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments. That looks neat!
.travis.yml
Outdated
stages: | ||
- name: test | ||
- name: release | ||
if: (branch = master AND type = push) OR (tag IS present) | ||
- name: upload-launcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the upload-launcher
stage? Couldn't it be kept along the new upload-artifacts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because, if my understanding of travis is correct, jobs run only in parallel if they're in the same stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put the upload-launcher
stage back in as it was before.
Dockerfile
Outdated
# i.e SCALA_VERSIONS="2.11.12 2.12.8" | ||
ARG SCALA_VERSIONS | ||
USER $NB_UID | ||
RUN [ -z "$SCALA_VERSIONS" ] && echo "SCALA_VERSIONS is empty" && exit 1; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT Maybe we could put that command in a script, to make it easier to tweak it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. That would also make it easier to reuse it outside of a docker build. I'll try to move it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
scripts/update-docker-images.sh
Outdated
git add Dockerfile | ||
git commit -m "almond $VERSION, scala $sv" | ||
done | ||
sbt '+ publishLocal' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to do that for releases? They should already be on Maven Central when this stage runs.
Maybe we could run that stage for snapshots (and push them with a snapshot
tag say), and rely on publishLocal
only for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it runs in parallel to the upload-launcher (see my comment about the stage) so the artifacts aren't available yet. I can change it back to run afterwards and use the published artifacts. Then only do a publishLocal
for snapshots builds as you've suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can run them sequentially, yes. Doing so allows to avoid the +publishLocal
, so maybe it's not even slower…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script in combination with .travis.yml differentiates now between releases and a push to master. +publishLocal
runs only on master now and creates a snapshot docker image from the current HEAD. On a tag, it should grab the published artifacts instead.
- build snapshots only when pushing to master, not when pushing a tag - move docker job back into its own stage
.travis.yml
Outdated
@@ -18,7 +18,7 @@ stages: | |||
- name: release | |||
if: (branch = master AND type = push) OR (tag IS present) | |||
- name: upload-launcher | |||
if: (branch = master AND type = push) OR tag IS present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I this change was supposed to be applied to the update-docker-images stage in order to support snapshot images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, np!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad, I opened #343 to re-instate it.
Edit: the condition of the docker stage wasn't changed, so it's still only built for releases. |
So merging, thanks! |
The docker stage fails because there's a versioning issue. Cause by this linein update-docker-images.sh because on travis it uses git versioning. |
I guess the most robust fix would be to get the version out of sbt somehow. |
We can always set the version right before the |
This is an alternative approach to almond-sh/docker-images#1.
The main differences are:
The Dockerfile makes it easy to create a docker image from a locally built kernel by setting
LOCAL_IVY=yes
. This is used in travis where the images are created from locally published jars, which is why we can run this job in parallel to uploading to sonatype.Right now it does a
sbt + publishLocal
in the update-docker-images script, then copies$HOME/.ivy/local
into$TRAVIS_BUILD_DIR/ivy-local
so we can copy it into the docker image. I think this could be improved by doing an additional publish to$TRAVIS_BUILD_DIR/ivy-local
during the release stage directly. I tried but my understanding of sbt seems to be too limited.